Conversation
with crosscompile, distroless and opencontainers label
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes consolidate the Docker build process by removing multi-architecture binary pre-builds from CI workflows and eliminating Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
94-109: ✗ Missing unzip dependency in update_os target.The
install_xraytarget relies onscripts/install_xray.sh, which callsunzip -qon line 93. However, none of the distribution-specific branches inupdate_os(lines 68–88) install unzip—only curl and bash are installed. This will cause the script to fail with "unzip: not found" when invoked on any supported Linux distribution.Apply this diff to add unzip to all update_os install commands:
# Debian/Ubuntu if [ "$(DISTRO)" = "debian" ] || [ "$(DISTRO)" = "ubuntu" ]; then \ - sudo apt-get update && \ - sudo apt-get install -y curl bash; \ + sudo apt-get update && \ + sudo apt-get install -y curl bash unzip; \ fi # Alpine Linux if [ "$(DISTRO)" = "alpine" ]; then \ apk update && \ - apk add --no-cache curl bash; \ + apk add --no-cache curl bash unzip; \ fi # CentOS/RHEL/Fedora if [ "$(DISTRO)" = "centos" ] || [ "$(DISTRO)" = "rhel" ] || [ "$(DISTRO)" = "fedora" ]; then \ sudo yum update -y && \ - sudo yum install -y curl bash; \ + sudo yum install -y curl bash unzip; \ fi # Arch Linux if [ "$(DISTRO)" = "arch" ]; then \ - sudo pacman -Sy --noconfirm curl bash; \ + sudo pacman -Sy --noconfirm curl bash unzip; \ fi
🧹 Nitpick comments (2)
scripts/install_xray.sh (1)
32-50: CPU feature detection is fragile; consider defensive guards.Lines 33 and 37 rely on
/proc/cpuinfoto detect thevfp(vector floating point) feature for ARM variants. Lines 50 useslscputo detect endianness for MIPS64. Both approaches have portability risks:
/proc/cpuinfoformat and available fields vary by architecture and environment (e.g., QEMU, containers)lscpumay not be available on minimal systems or inside lightweight containersWhile the fallback to
arm32-v5(lines 33, 37) is conservative, the script should either verify these tools exist before use or document the assumptions. Consider adding checks or fallback behavior if these commands fail.Optionally, consider adding defensive checks:
# Line 33 area - add guard for cpuinfo check if [ -f /proc/cpuinfo ] && grep -q Features /proc/cpuinfo; then grep Features /proc/cpuinfo | grep -qw 'vfp' || ARCH='arm32-v5' fi # Line 50 area - add guard for lscpu if command -v lscpu >/dev/null 2>&1; then lscpu | grep -q "Little Endian" && ARCH='mips64le' fiDockerfile (1)
22-24: Optimize COPY to reduce final image bloat.Line 22 copies the entire builder stage's
/srcdirectory (including source code, scripts, and build artifacts) into the final distroless image. Distroless images are designed for minimal attack surface and small size. Copying unnecessary build artifacts and source code negates some of these benefits.Consider copying only the essential runtime artifacts:
-COPY --from=builder /src /app +COPY --from=builder /src/main /app/mainThen separately copy only the runtime config or data files if needed (e.g., any config.json, data files, etc.). This keeps the final image focused on runtime dependencies only.
Verify which files in
/srcare actually needed at runtime. If only the binary is needed, the narrower COPY is preferred. If config files or other data are required, copy them explicitly rather than the entire directory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/docker-build-dev.yml(1 hunks).github/workflows/docker-build.yml(1 hunks)Dockerfile(1 hunks)Dockerfile.multi(0 hunks)Makefile(1 hunks)scripts/install_xray.sh(1 hunks)
💤 Files with no reviewable changes (1)
- Dockerfile.multi
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (3)
.github/workflows/docker-build-dev.yml (1)
42-42: ✓ Workflow simplification looks good.Moving the build process into the multi-stage Dockerfile and eliminating pre-built cross-compiled binaries in the CI workflow is a clean simplification. The use of Docker Buildx with native Go cross-compilation (via GOOS/GOARCH) is the modern, preferred approach.
.github/workflows/docker-build.yml (1)
61-61: ✓ Consistent workflow update for release builds.Aligns with the dev workflow change to reference a single multi-stage Dockerfile instead of Dockerfile.multi. The release workflow correctly delegates build logic to the Dockerfile.
Dockerfile (1)
14-15: The review comment is based on incorrect assumptions and does not apply to the actual codebase.The original comment references a non-existent
scripts/install_xray.shfile and quotes line 93 from it. In reality:
- No local
scripts/install_xray.shexists in the repository.- The Makefile's
install_xraytarget (lines 94–108) downloads and executes an external script directly from GitHub (https://github.com/Gozargah/Marzban-scripts/raw/master/install_latest_xray.sh), not a local script.- The claim about an
unzipdependency is unfounded—there is no local script to examine.The actual Dockerfile (line 23) does execute
make install_xrayin an Alpine container, which has legitimate concerns (external script execution, curl dependency), but they differ from those claimed in the review.Likely an incorrect or invalid review comment.
scripts/install_xray.sh
Outdated
| #!/usr/bin/env bash | ||
|
|
||
| # Download Xray latest | ||
|
|
||
| RELEASE_TAG="latest" | ||
|
|
||
| if [[ "$1" ]]; then | ||
| RELEASE_TAG="$1" | ||
| fi | ||
|
|
There was a problem hiding this comment.
Add set -e to ensure Docker build fails on any error.
The script lacks set -e at the top, which means if download_xray fails (returns 1), execution continues to extract_xray. Inside Docker (line 15 of Dockerfile: RUN make install_xray), a failure in a sub-function might not propagate cleanly to stop the build. In the worst case, the build could silently proceed with incomplete artifacts.
Add set -e after the shebang:
#!/usr/bin/env bash
+set -e
# Download Xray latest📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #!/usr/bin/env bash | |
| # Download Xray latest | |
| RELEASE_TAG="latest" | |
| if [[ "$1" ]]; then | |
| RELEASE_TAG="$1" | |
| fi | |
| #!/usr/bin/env bash | |
| set -e | |
| # Download Xray latest | |
| RELEASE_TAG="latest" | |
| if [[ "$1" ]]; then | |
| RELEASE_TAG="$1" | |
| fi |
🤖 Prompt for AI Agents
In scripts/install_xray.sh around lines 1 to 10, the script is missing a safety
flag so failures in functions (like download_xray) don't allow execution to
continue; add "set -e" immediately after the shebang (#!/usr/bin/env bash) so
the script exits on any command failure, ensuring Docker builds fail fast when a
step errors.
|
Thanks for pr |
That’s exactly why native cross-compilation was used — so we don’t waste CPU power on QEMU emulation.
Distroless uses Debian only as a source for system libraries. In practice, Distroless isn’t a real OS: “Distroless images contain only your application and its runtime dependencies. They do not contain package managers, shells, or any other programs you would expect to find in a standard Linux distribution.” See more here. The binary build process still uses Alpine. |
|
let's see how it will affect image build process |
|
well it made the build time double instead of optimizing it |
|
Workflow sent cache for 140 seconds: Last time there weren't many changes, so he only sent 14 seconds: |
|
i re-run the workflow and now reached 53 second i think the reason cache takes this long is because of additional layers new docker file have probably i bring back Dockerfile.multi but with debian and see what happens then, probably this will be most efficient way |
|
i was facing a execute error, also we need to use alpine for future cases, one of them is wireguard support |
2 participants
This PR optimizes the Dockerfile and related workflows, reducing the final image size by ~17% and narrowing the attack surface. Key points:
docker-buildanddocker-build-devworkflows no longer build the binary. The build process is now fully handled inside the Dockerfile.Dockerfile.multihas been removed in favor of a singleDockerfile. I don’t fully understand the original reason for this separation, so if it was important, please explain the purpose and I will try to adapt the PR accordingly (or you can adjust it yourselves).static-debian12). Distroless reduces image size and lowers the attack surface (as also confirmed by Docker Scout analysis).scriptsdirectory. Previously it lived in the Marzban repository, which is no longer maintained, so for safety reasons I decided to relocate it here.install_xray.Additionally, my initial intention was simply to switch the final image from Alpine to Distroless, but after reviewing the CI setup and seeing the legacy Marzban scripts in use, I ended up doing a bit more work in this PR than originally planned :)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.